Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Padding iphone x #1007

Merged
merged 3 commits into from Aug 27, 2020
Merged

Padding iphone x #1007

merged 3 commits into from Aug 27, 2020

Conversation

ronderksen
Copy link
Contributor

Closes #782

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

Open the picker examples on an iPhone X or newer (without a physical home button) in portrait mode and try to tap the last item.
The solution still isn't perfect, but it seems to at least clear the home bar at the bottom of the screen

🧢 Your Project:

None

@@ -66,6 +66,10 @@
transition: opacity var(--spectrum-dialog-exit-animation-duration) cubic-bezier(0.5, 0, 1, 1) var(--spectrum-dialog-exit-animation-delay),
transform var(--spectrum-dialog-exit-animation-duration) cubic-bezier(0.5, 0, 1, 1) var(--spectrum-dialog-exit-animation-delay);

&:last-child {
padding-bottom: calc(env(safe-area-inset-bottom) + 8px);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while a really neat function that i just learned about (thank you!)
it seems it relies on a specific meta tag? https://developer.mozilla.org/en-US/docs/Web/CSS/env

To tell the browser to use the whole available space on the screen, and so enabling us to use the env() variables, we need to add a new viewport meta value:

is this not the case?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm well we don't have viewport-fit=cover in the docs site, just width=device-width, initial-scale=1 and we still have this issue...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need the meta tag part. I've checked and I definitely see a difference in padding with and without the env() part. But I will triple check to be sure.

Also, tray is always the last child of the tray wrapper element, so always adding the padding is fine.
@devongovett
Copy link
Member

Thanks @ronderksen! I've removed the extra 8px of padding in addition to the safe area inset because that would affect non-iPhones and also when the Safari toolbar is visible. Also, the tray is always the last child of the tray wrapper element, so always adding the padding is fine.

I assume the extra 8px was to try to avoid tapping near the bottom showing the toolbar and hiding the tray due to scrolling. I'm going to open a separate PR to prevent that.

@devongovett devongovett merged commit 3934bfe into adobe:main Aug 27, 2020
@devongovett
Copy link
Member

#1023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trays need extra padding on iPhone X
3 participants